Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix RuntimeError in export_phrases (change defaultdict to dict) #3041

Merged
merged 9 commits into from
Feb 13, 2021
Merged

Fix RuntimeError in export_phrases (change defaultdict to dict) #3041

merged 9 commits into from
Feb 13, 2021

Conversation

thalishsajeed
Copy link
Contributor

@thalishsajeed thalishsajeed commented Feb 7, 2021 •
edited
Loading

Fixes #3031

This PR -

  • Replaces defaultdict with dict in phrases.py to avoid the Runtime error caused when defaultdict modifies its contents during runtime each time it encounters an out of vocabulary token.
  • Adds test cases to check for above bug

@thalishsajeed
Copy link
Contributor Author

@piskvorky Hi, hope I've done everything right this time :) Thank you for all the help!

@piskvorky
Copy link
Owner

piskvorky commented Feb 7, 2021 •
edited
Loading

Use a better PR title and motivation in the description please.

And also link from here to the previous PR, where most of the discussion and review happened.

piskvorky
piskvorky approved these changes Feb 7, 2021
View reviewed changes
Copy link
Owner

@piskvorky piskvorky left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, thanks! But fix the PR title + description please. We use those to generate Release Notes, and auto-close affected tickets on merge.

gensim/models/phrases.py Outdated Show resolved Hide resolved
@thalishsajeed thalishsajeed changed the title Fix #3031 Replace defaultdict with dict in phrases.py to fix Runtime error in while using export_phrases() Feb 7, 2021
@thalishsajeed thalishsajeed changed the title Replace defaultdict with dict in phrases.py to fix Runtime error in while using export_phrases() Replace defaultdict with dict in phrases.py to fix Runtime error while using export_phrases() Feb 9, 2021
@mpenkov mpenkov changed the title Replace defaultdict with dict in phrases.py to fix Runtime error while using export_phrases() fix RuntimeError in export_phrases (change defaultdict to dict) Feb 13, 2021
@mpenkov
Copy link
Collaborator

mpenkov commented Feb 13, 2021

Merged. Thank you @thalishsajeed and congrats on your first contribution to gensim!

@mpenkov mpenkov merged commit cfc9e95 into piskvorky:develop Feb 13, 2021
@thalishsajeed
Copy link
Contributor Author

@mpenkov Thank you! :) Looking forward to gensim 4.0

@piskvorky piskvorky changed the title fix RuntimeError in export_phrases (change defaultdict to dict) Fix RuntimeError in export_phrases (change defaultdict to dict) Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpenkov mpenkov mpenkov approved these changes

@piskvorky piskvorky piskvorky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Runtime error in phrases.py
3 participants
@thalishsajeed @piskvorky @mpenkov